Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zmq RPC in daemon #2044

Merged
merged 4 commits into from Sep 18, 2017
Merged

Zmq RPC in daemon #2044

merged 4 commits into from Sep 18, 2017

Conversation

tewinget
Copy link
Contributor

@tewinget tewinget commented May 24, 2017

There are things that could still be improved, but I think it's in a good state to be merged.

EDIT: separated some changes into a different PR, now this PR depends #2184.

@@ -91,7 +91,7 @@ namespace cryptonote

struct txin_gen
{
size_t height;
uint64_t height;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted, also the three ones below (as per my IRC explanation)


uint64_t Blockchain::get_num_mature_outputs(uint64_t amount) const
{
auto num_outs = m_db->get_num_outputs(amount);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general comment, I prefer when types aren't left to "whatever" for integer types, where size and signedness matter a lot. This should be uint64_t, but it hidden here.

@@ -567,6 +576,15 @@ namespace cryptonote
//transaction is ok.
return true;
}

//---------------------------------------------------------------------------------
void tx_memory_pool::get_transactions_and_key_images(transactions_container& txs, key_images_container& key_images) const
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent got wonky here

@@ -514,6 +514,15 @@ namespace cryptonote
return m_spent_key_images.end() != m_spent_key_images.find(key_im);
}
//---------------------------------------------------------------------------------
void tx_memory_pool::have_key_images_as_spent(const std::vector<crypto::key_image>& key_images, std::vector<bool>& spent) const
{
CRITICAL_REGION_LOCAL(m_transactions_lock);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this gets merged after the txpool patch, the blockchain lock needs taking here after the transactions lock

void tx_memory_pool::get_transactions_and_key_images(transactions_container& txs, key_images_container& key_images) const
{
// need to lock so the transactions container doesn't change during copy
CRITICAL_REGION_LOCAL(m_transactions_lock);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this gets merged after the txpool patch, the blockchain lock needs taking here after the transactions lock

zmq::socket_t* socket = new zmq::socket_t(context, ZMQ_REP);

std::string bind_address = addr_prefix + address + std::string(":") + port;
socket->bind(bind_address.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error checking ?

if (new_socket)
{
delete new_socket;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deletes on exception, but the previous function does not. Different semantics, or leak ?

return false;
}
return true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those two functions should be merged, they're pretty much the same.

running = false;

return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks racy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

}

i = (uint8_t)( val.GetUint() & 0xFF);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd error out on bad range, instead of losing bits. Less scope for confusion, exploits, etc.

@fluffypony
Copy link
Contributor

Needs rebasing, and check review comments

@peronero
Copy link

peronero commented Jun 10, 2017

I hate to be that guy, but what's going on here? Back-and-forth turnaround is seemingly on weeks-to-months timeframes and @tewinget seldom attends the dev meetings, despite this being under the FFS. The funding proposal is sparse in detail but this work seems to be more than 6 months late by the original estimate.

This is a pretty significant component - as I understand it, a hard dependency for functionality like block- and wallet- notify and in turn a roadblock for ecosystem infrastructure development.

I would recommend either:

1 - getting committment from tewinget to complete this work on some sort of firm timetable
2 - having someone familiar with the codebase (like @moneromooo-monero ) estimate the remaining effort and reallocating the difference from the 1000XMR left to another developer

There is, ostensibly, a dev meeting tomorrow where this can be discussed.

@tewinget
Copy link
Contributor Author

tewinget commented Jun 10, 2017 via email

@dternyak
Copy link

@tewinget Thank you for your responsible and courteous reply, as well as the commitment to have ZMQ ready by the end of the month.

I have been following ZMQ's development for a while now, and I, among many others, are excited that it's getting close 👍

@ghost
Copy link

ghost commented Jun 10, 2017

@tewinget Thanks

@peronero
Copy link

Thanks @tewinget.

If I could make one more recommendation - and I really hate to be that guy again - but you might find it useful to break down the work into smaller milestones instead of aiming to deliver the whole thing 3 weeks from now.

Cheers

@tewinget
Copy link
Contributor Author

tewinget commented Jun 11, 2017 via email

@tewinget
Copy link
Contributor Author

Apart from squashing any compiler bugs on various platforms and choosing a version of libzmq to bring in-source and doing that (assuming we still want to do so), I believe all of the comments here have been addressed. Testing is a bit tedious until I've got the client side of things back in good shape -- look for that in the next two days. If you're dying to test things sooner than that, PM me on IRC and I'll get back to you as soon as I see the message.

@peronero
Copy link

peronero commented Jul 1, 2017

OK, so I believe we're in a spot where these components can be reviewed and iterated upon if need be, and the notify stuff is impending separately.

Thanks @tewinget

res.blocks.clear();
res.output_indices.clear();
res.status = Message::STATUS_FAILED;
res.error_details = "failed retrieving a requested block";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transaction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough; I put block because it's in the get blocks rpc, but tx makes sense, context should be known.

for (const crypto::hash& h : bwt.block.tx_hashes)
{
bwt.transactions.emplace(h, *tx_it);
tx_it++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should still error out if tx_it goes to end()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted.

res.info.grey_peerlist_size = m_p2p.get_peerlist_manager().get_gray_peers_count();

res.info.testnet = m_core.get_testnet();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing various fields added in the last year

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go through all the calls to make sure they're up to date, and get rid of the deprecated ones.

{
res.status = Message::STATUS_FAILED;
res.error_details = "RPC method not yet implemented.";
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all remnants of fast exit were nuked ages ago

Copy link

@ghost ghost Jul 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. #1171

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go through all the calls to make sure they're up to date, and get rid of the deprecated ones.


void DaemonHandler::handle(const GetRPCVersion::Request& req, GetRPCVersion::Response& res)
{
res.version = DAEMON_RPC_VERSION;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be renamed wiht 0MQ in the name or it'll get confused with the existing one (as I just did).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

outputs_for_amount outputs;
};

struct peer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to shed ip assumption, see the new network_address class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the wallet doesn't use that RPC call, I hadn't touched it up yet. Will deal with that when I go through to check all the other calls I've implemented for changes.


while (1)
{
if (rep_socket)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this will busy eat the CPU is rep_socket is false/NULL.

zmq::message_t reply(response.size());
memcpy((void *) reply.data(), response.c_str(), response.size());

rep_socket->send(reply);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No error code ?

Copy link
Contributor Author

@tewinget tewinget Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpp bindings for zmq throw, catching now.

On that note, not sure if best course is to re-throw upon catching and error messaging, or silently ignore. Will have to research a little to see how resilient it is.

std::string bind_address = addr_prefix + address + std::string(":") + port;
rep_socket->bind(bind_address.c_str());
}
catch (std::exception& e)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you missed one about 30 lines above that ;)

throw WRONG_TYPE("integer");
}

if (val.GetInt() > 0xFF)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7f (and negative bound too at... -80 I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, there's definitely a less clumsy but imo uglier way of doing this. Will sort it simply for now and look into a cleaner solution later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-worked this logic in a PR issued to @tewinget repo/branch. The limits are now checked using numeric_limits<...>min/max.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted; reviewed on my repo.

@ghost
Copy link

ghost commented Jul 10, 2017

Can't some of the more general fixes (e.g. to blockchain.cpp) be submitted as separate PRs, leaving only the ZMQ RPC stuff to go in?

@tewinget
Copy link
Contributor Author

tewinget commented Jul 11, 2017 via email

@moneromooo-monero
Copy link
Collaborator

NanoAkron was not asking to remove them, but to PR them separately. Which is a good idea usually (at least commit them separately, which is what I try to do). With this particular massive patch set though, I think it's past the point where it can be made to look nice anyway. Let it be the last one to be ^_^, and let's get it merged once the missing recent (and not so recent) changes are also in the 0MQ RPC :)

@tewinget
Copy link
Contributor Author

tewinget commented Jul 11, 2017 via email

@peronero
Copy link

peronero commented Jul 21, 2017

@fluffypony @luigi1111 @iamsmooth @moneromooo-monero

It's 3 weeks into the next month and 10 days since last message from @tewinget and this is still not merged.

I will not be here for the dev meeting but if not merged by then I strongly recommend that the community finds someone else to complete this work as per the 2nd option above - billing against the remaining funds from this FFS and disbursing the difference, if any, tewinget's way.

It's ridiculous that volunteers have to chase down what is effectively a $2000/hr developer to complete work that is more than half a year late.

@ghost
Copy link

ghost commented Jul 23, 2017

Could you squash commits to remove things like 'macros, yay' please?

@barnyardanimal
Copy link

barnyardanimal commented Jul 31, 2017

This would be a lot easier to review if broken into smaller pieces. Can we have another update from @tewinget on his effort to clean things up?

@tewinget
Copy link
Contributor Author

tewinget commented Aug 1, 2017 via email

@moneromooo-monero
Copy link
Collaborator

I think cleaning up commits helps a lot, because you then have N commits of size X, instead of one large amorphous blob of size N*X, because it can't really be considered small parts. That said, I'm ready to accept this PR as a large set of disparate commits, as long as it's the last one. But some cleanup would be most welcome (especially rolling any fixes/cleanups back into the commit which introduced them if that applies here).

@tewinget
Copy link
Contributor Author

tewinget commented Aug 1, 2017 via email

@expez
Copy link

expez commented Aug 12, 2017

I would recommend either:

1 - getting committment from tewinget to complete this work on some sort of firm timetable
2 - having someone familiar with the codebase (like @moneromooo-monero ) estimate the remaining effort and reallocating the difference from the 1000XMR left to another developer

Two months ago we tried 1).

Would you be OK with giving 2) a try @tewinget?

@tewinget
Copy link
Contributor Author

tewinget commented Aug 12, 2017 via email

@dternyak
Copy link

@tewinget It appears there are a few outstanding comments by @moneromooo-monero that have gone unaddressed.

Are you planning on addressing these?

@tewinget
Copy link
Contributor Author

tewinget commented Aug 25, 2017 via email

@dternyak
Copy link

Great -- thank you.

@ghost
Copy link

ghost commented Sep 3, 2017

It appears the last commit was about 10 weeks ago. Is this in a state to merge? The 1000XMR is now around a quarter of a million dollars...

@tewinget
Copy link
Contributor Author

tewinget commented Sep 4, 2017 via email

@ghost
Copy link

ghost commented Sep 4, 2017

@tewinget Good to hear! Have you addressed all of @moneromooo-monero's concerns above though? There still seem to be 6 open discussions - would you at least comment on them so we can hear your thoughts?

@tewinget
Copy link
Contributor Author

tewinget commented Sep 4, 2017 via email

This commit refactors some of the rpc-related functions in the
Blockchain class to be more composable.  This change was made
in order to make implementing the new zmq rpc easier without
trampling on the old rpc.

New functions:
  Blockchain::get_num_mature_outputs
  Blockchain::get_random_outputs
  Blockchain::get_output_key
  Blockchain::get_output_key_mask_unlocked

  Blockchain::find_blockchain_supplement (overload)

functions which previously had this functionality inline now call these
functions as necessary.
Structured {de-,}serialization methods for (many new) types
which are used for requests or responses in the RPC.

New types include RPC requests and responses, and structs which compose
types within those.

# Conflicts:
#	src/cryptonote_core/blockchain.cpp
- Add some RPC commands (and touch up a couple others)
- some bounds checking
- some better pointer management
- const correctness and error handling

-- Thanks @vtnerd for type help with serialization and CMake changes
@tewinget
Copy link
Contributor Author

tewinget commented Sep 5, 2017

@NanoAkron thanks for reminding me I hadn't replied to those comments. I have now.

@moneromooo-monero
Copy link
Collaborator

I'd like this to be merged shortly after we are sure the release isn't borked.

Copy link
Contributor

@fluffypony fluffypony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

@fluffypony fluffypony merged commit 0299cb7 into monero-project:master Sep 18, 2017
fluffypony added a commit that referenced this pull request Sep 18, 2017
0299cb7 Fix various oversights/bugs in ZMQ RPC server code (Thomas Winget)
7798602 json serialization for rpc-relevant monero types (Thomas Winget)
5c1e08f Refactor some things into more composable (smaller) functions (Thomas Winget)
9ac2ad0 DRY refactoring (Thomas Winget)
@hyc
Copy link
Collaborator

hyc commented Sep 18, 2017

A few other problems with this PR. The code now depends on libzmq and a C++ wrapper for it <zmq.hpp> but there is no corresponding CMake test for this. Also this dependency needs to be listed in the README.md. As of now the build fails without explanation on various systems that don't have zmq installed by default. (E.g. #2471 )

@danrmiller
Copy link
Contributor

Do these undefined references in libzmq.a when using the wrapper from https://github.com/zeromq/cppzmq pretty much mean the bindings don't match the libs in the packages? Or something else?

I'm sure lots of users would prefer to be able to use the libs packaged by their OS, so I hope that will end up working.

https://build.getmonero.org/builders/monero-static-dragonflybsd-amd64/builds/1346/steps/compile/logs/stdio
https://build.getmonero.org/builders/monero-static-ubuntu-amd64/builds/2328/steps/compile/logs/stdio
https://build.getmonero.org/builders/monero-static-win64/builds/2280/steps/compile/logs/stdio

@tewinget
Copy link
Contributor Author

tewinget commented Sep 19, 2017 via email

@danrmiller danrmiller mentioned this pull request Sep 20, 2017
@jtgrassie
Copy link
Contributor

Sorry but latest (4.2.2) does not seem to include the cpp bindings (as installed via MacPorts and Homebrew on OSX). For OSX you have to install cppzmq too.

@Sexual
Copy link

Sexual commented Sep 25, 2017

Does this also add support for ZMQ pub/sub such as bitcoin's zmqpubhashtx. I couldn't seem to find any references to it in the code, so I'm doubtful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet